-
Notifications
You must be signed in to change notification settings - Fork 320
Add support to entrypoint v0.07 useroperations #2692
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. Weβll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
β¦cept UserOperationV7
β¦n_entrypoint_V07
config/settings/base.py
Outdated
| SAFE_4337_MODULE_ADDRESS_V06 = "0xa581c4A4DB7175302464fF3C06380BC3270b4037" | ||
| SAFE_4337_MODULE_ADDRESS_V07 = "0x75cf11467937ce3F2f357CE24ffc3DBF8fD5c226" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Being picky:
| SAFE_4337_MODULE_ADDRESS_V06 = "0xa581c4A4DB7175302464fF3C06380BC3270b4037" | |
| SAFE_4337_MODULE_ADDRESS_V07 = "0x75cf11467937ce3F2f357CE24ffc3DBF8fD5c226" | |
| ETHEREUM_4337_SAFE_MODULE_ADDRESS_V06 = "0xa581c4A4DB7175302464fF3C06380BC3270b4037" | |
| ETHEREUM_4337_SAFE_MODULE_ADDRESS_V07 = "0x75cf11467937ce3F2f357CE24ffc3DBF8fD5c226" |
So all the settings related to 4337 start the same.
Also hardcoding shouldn't be done, it needs to be defined using environment configuration variables as the other configuration (using env.str)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok
| user_operation_metadata, | ||
| ) | ||
| else: | ||
| raise ValueError("Unsupported entrypoint") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do entrypoints have the same address for every network? Remember that we run tx service for a lot of different networks, and it needs to be compatible with them.
There's no other way to tell appart v6 from v7? I would prefer to check a field that exists on v7 and not on v6, for example. Checking entrypoint address is a good solution for your use case, but not for the service
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All the 4337 tooling that i know of assumes the official entrypoints addresses.
eth-infinitism/account-abstraction#372 (comment)
I think it is safe to only support the official entrypoints addresses.
Because we are using the same class for both useroperation types, a useroperation without init/factory and without paymaster would be identical for both v6 and v7(except for the entrypoint address).
Also v7, v8 and v9 have the same useroperation structure.
So i think using the entrypoint address is the only option with the current structure.
I added some model constrains as sanity checks to verify that some values should be none for each useroperation type, i can repeat these sanity checks here too.
| from safe_eth.eth.utils import fast_keccak | ||
| from safe_eth.safe.account_abstraction import SafeOperation as SafeOperationClass | ||
|
|
||
| from safe_transaction_service.account_abstraction.UserOperationV7 import UserOperationV7 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
UserOperationV7 should be part of safe-eth-py. I understand for a draft is ok to be here, but for the merge both classes should be together in safe-eth-py (I will review that PR, just let me know π )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok
| ENTRYPOINT_V6 = "0x5FF137D4b0FDCD49DcA30c7CF57E578a026d2789" | ||
| ENTRYPOINT_V7 = "0x0000000071727De22E5E9d8BAf0edAc6f37da032" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No hardcoding, read this from base.py
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok
|
|
||
| entry_point = attrs["entry_point"] | ||
| module_address = attrs["module_address"] | ||
| is_v7 = entry_point.lower() == ENTRYPOINT_V7.lower() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, please don't use entrypoint address and find another alternative like fields even if it's not so "correct"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both useroperation types use the same class, so they can be identical(except for the entrypoint address). I can add some sanity checks.
| self.validated_data["entry_point"], | ||
| ) | ||
| entry_point = self.validated_data["entry_point"] | ||
| is_v7 = entry_point.lower() == ENTRYPOINT_V7.lower() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Samewise
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same
| # Remove version-specific fields based on entrypoint | ||
| data = super().to_representation(instance) | ||
|
|
||
| is_v7 = instance.entry_point.lower() == ENTRYPOINT_V7.lower() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same. Also, I see this comparison in a lot of places. Maybe it can be refactored to a util function like "def is_v7(...) -> bool`
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same
| from ..models import SafeOperationConfirmation as SafeOperationConfirmationModel | ||
| from ..models import UserOperation as UserOperationModel | ||
| from ..models import UserOperationReceipt as UserOperationReceiptModel | ||
| from ..SafeOperation import SafeOperation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
filenames and packages should be snake case in Python
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok
|
Thank you for taking the time to review this pr. |
Co-authored-by: UxΓo <[email protected]>
β¦rom mock_bundler
|
some tests are failing, not sure if these are related to this pr or due to compatibility with safe-eth-py 7.17.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR is being reviewed by Cursor Bugbot
Details
Your team is on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle for each member of your team.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
| self.validated_data["paymaster_verification_gas_limit"], | ||
| self.validated_data["paymaster_post_op_gas_limit"], | ||
| self.validated_data["paymaster"], | ||
| self.validated_data.get["paymaster_data"] or b"", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Method call syntax error using brackets instead of parentheses
The code uses self.validated_data.get["paymaster_data"] with square brackets instead of parentheses. This attempts to subscript the .get method rather than calling it, causing a TypeError at runtime when saving v7 user operations. The correct syntax is self.validated_data.get("paymaster_data").
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sherifahmed990 sorry for the delay, as I was on holidays. Could you take a look at this bug?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
| self.validated_data["factory_data"] or b"", | ||
| self.validated_data["paymaster_verification_gas_limit"], | ||
| self.validated_data["paymaster_post_op_gas_limit"], | ||
| self.validated_data["paymaster"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing paymaster field causes KeyError for v7 operations
The code accesses self.validated_data["paymaster"] but SafeOperationSerializer does not define a paymaster field. The serializer only defines paymaster_and_data for v6 and paymaster_verification_gas_limit/paymaster_post_op_gas_limit for v7 operations. This will cause a KeyError when attempting to save a v7 user operation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I used Claude to generate a test file for SafeOperationSerializer, I can remove it if it is not relevant to this pr.
| self.validated_data["verification_gas_limit"], | ||
| self.validated_data["pre_verification_gas"], | ||
| self.validated_data["max_priority_fee_per_gas"], | ||
| self.validated_data["max_fee_per_gas"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Swapped argument order for gas fee parameters in v7
The v7 UserOperationV7 constructor in save() passes max_priority_fee_per_gas before max_fee_per_gas, but the v6 constructor (and models.py to_user_operation() for both versions) passes them in the opposite order: max_fee_per_gas first. This argument order inconsistency will cause incorrect user operation hash calculation, resulting in signature validation failures or hash mismatches.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, approved
β¦_until Tests cover validation of: - Module address and entry point version matching - v6/v7 field compatibility (factory, paymaster fields) - Nonce validation - Signature validation - Paymaster and paymaster_data validation - valid_after and valid_until timestamp validation Co-Authored-By: Claude Opus 4.5 <[email protected]>
Make sure these boxes are checked! π¦β
./run_tests.shpre-commit run -aWhat was wrong? πΎ
This draft pr adds support to entrypoint v0.07 useroperations.
I wanted to get an approval about the structure of the pr, for the following:
1- Adding more fields to the useroperation table instead of creating a new table.
2-Creating a modified version of UserOperationV7 instead of UserOperationV07 to allow for optional factory, factory_data and adding init_code property method.
3- Creating a modified version of SafeOperation to override from_user_operation.
then I will add tests for v0.07 useroperations
@Uxio0
Note
Adds EntryPoint v0.7 support across Account Abstraction, while keeping v0.6 compatibility and gating by
ETHEREUM_4337_SUPPORTED_ENTRY_POINTS.ETHEREUM_4337_ENTRYPOINT_V6/V7and Safe module addresses; default-support both viaETHEREUM_4337_SUPPORTED_ENTRY_POINTSandETHEREUM_4337_SUPPORTED_SAFE_MODULES.UserOperationwith v0.7 fields (factory,factory_data,paymaster_verification_gas_limit,paymaster_post_op_gas_limit) and add constraints;to_user_operationnow returnsUserOperationV6orUserOperationV7based onentry_point.UserOperation(v6/v7); response strips non-applicable fields; confirmations decode init data from v7 (factory+factory_data) when present.entry_pointis insupported_entry_points; persist v7-specific fields when indexing; remove blanket v0.7 rejection.Written by Cursor Bugbot for commit c2e4b16. This will update automatically on new commits. Configure here.